Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves observability for theme file watching by surfacing Chokidar watcher errors via logging and runtime analytics, helping diagnose reports of files not syncing.
Changes:
- Add a Chokidar
.on('error')handler to log watcher errors and emit an analytics event. - Add a unit test covering watcher error handling and analytics emission.
- Mock
recordEventin tests to assert telemetry behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/theme/src/cli/utilities/theme-fs.ts | Adds watcher error handler that logs and records an analytics event. |
| packages/theme/src/cli/utilities/theme-fs.test.ts | Adds test coverage for watcher error emission and mocks analytics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .on('unlink', queueFsEvent.bind(null, 'unlink')) | ||
| .on('error', (error) => { | ||
| outputDebug(`File watcher error: ${error}`) | ||
| recordEvent('theme-service:file-watcher:error') |
There was a problem hiding this comment.
Should we use recordError, too?
There was a problem hiding this comment.
Yeah I think that's probably a better idea to switch from event to error. The event list is time stampped so it will be in order regardless of which record option we choose.
It makes more sense to record it as an error like some of our other areas.
EvilGenius13
force-pushed
the
jf-on-error
branch
from
52a1d38 to
1fbf61b
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/system.d.ts@@ -87,12 +87,6 @@ export declare function exec(command: string, args: string[], options?: ExecOpti
* @returns A Promise resolving after the number of seconds.
*/
export declare function sleep(seconds: number): Promise<void>;
-/**
- * Check if the terminal supports OSC 8 hyperlinks.
- *
- * @returns True if the terminal supports hyperlinks.
- */
-export declare function terminalSupportsHyperlinks(): boolean;
/**
* Check if the standard input and output streams support prompting.
*
packages/cli-kit/dist/public/node/ui.d.ts@@ -328,6 +328,7 @@ interface RenderTasksOptions {
/**
* Runs async tasks and displays their progress to the console.
* @example
+ * ▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀
* Installing dependencies ...
*/
export declare function renderTasks<TContext>(tasks: Task<TContext>[], { renderOptions, noProgressBar }?: RenderTasksOptions): Promise<TContext>;
@@ -345,6 +346,7 @@ export interface RenderSingleTaskOptions<T> {
* @param options.renderOptions - Optional render configuration
* @returns The result of the task
* @example
+ * ▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀
* Loading app ...
*/
export declare function renderSingleTask<T>({ title, task, onAbort, renderOptions, }: RenderSingleTaskOptions<T>): Promise<T>;
|
WHY are these changes introduced?
We have had some reports of files not syncing, and there's the possibility that the watcher is throwing an error we're silently swallowing.
WHAT is this pull request doing?
add a
.on('error')branch which will output a debug log and record it to our monorail events.How to test your changes?
I don't have a good way of replicating a real OS-level failure for this but feel free to run the test I added.
Post-release steps
Checklist
pnpm changeset add